fix(core): mirror Qwen reasoning history field#4289
Conversation
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Cross-file note: mirrorQwenReasoningContent adds reasoning field; MistralOpenAICompatibleProvider.buildRequest() calls super.buildRequest() (now running the mirror), then runs stripReasoningContent which removes reasoning_content but not reasoning. Low probability (requires Qwen model on Mistral endpoint) but if triggered, Mistral API may receive an orphaned reasoning field. Consider also stripping reasoning in stripReasoningContent.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| max_tokens: effectiveMaxTokens, | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] Missing JSDoc on mirrorQwenReasoningContent. The method silently transforms messages by copying reasoning_content to reasoning, but there is no comment explaining why this is needed. Other methods like applyOutputTokenLimit and DeepSeek's equivalent have JSDoc blocks explaining the motivation and linked issues.
| /** | |
| * Mirrors `reasoning_content` to `reasoning` for Qwen model history. | |
| * Qwen's self-hosted API requires the `reasoning` field (not just | |
| * `reasoning_content`) when replaying assistant messages in multi-turn | |
| * conversations. This copies the content without removing the original | |
| * field to preserve backward compatibility. | |
| * | |
| * Fixes #4285 | |
| */ | |
| private mirrorQwenReasoningContent( |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| }); | ||
| }); | ||
|
|
||
| it('should mirror reasoning_content to reasoning for Qwen model history', () => { |
There was a problem hiding this comment.
[Suggestion] The single test covers only the happy path (one assistant message with reasoning_content). Four critical branches are untested:
- Non-Qwen model no-op —
model: 'gpt-4'withreasoning_contentshould NOT injectreasoning - Empty
reasoning_content—reasoning_content: ''should skip mirroring (explicitlength === 0guard) - Existing
reasoningfield — assistant message with bothreasoningandreasoning_contentshould NOT overwritereasoning - Non-assistant messages — user/system messages with
reasoning_contentshould be ignored (role !== 'assistant'guard)
These guards are the most frequently hit code paths and regressions here would silently break API behavior.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| return message; | ||
| } | ||
|
|
||
| const extended = message as unknown as Record<string, unknown>; |
There was a problem hiding this comment.
[Suggestion] The double-cast as unknown as Record<string, unknown> to access reasoning_content can be replaced with the project's existing typed interface. ExtendedChatCompletionAssistantMessageParam (exported from converter.ts) already declares reasoning_content?: string | null. DeepSeek's equivalent function uses it directly:
| const extended = message as unknown as Record<string, unknown>; | |
| const extended = message as ExtendedChatCompletionAssistantMessageParam; | |
| const reasoningContent = extended.reasoning_content; |
This catches typos, provides IDE support, and is consistent with how ensureReasoningContentOnToolCalls in deepseek.ts accesses the same field. The write side (reasoning) still needs a cast since that field isn't in the type.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
LaZzyMan
left a comment
LaZzyMan
left a comment
There was a problem hiding this comment.
Review
The fix is correct in isolation — assistant-role-only walk, immutable spread, guard against overwriting an explicit reasoning, guard against empty reasoning_content. The same shape was merged on May 19 in PR #4294, and this branch hasn't been rebased. After rebase the mirror loop here would run on top of #4294's already-populated reasoning and no-op on every message, leaving the PR as duplicate dead code.
The one substantive difference is the model marker — 'qwen' here, 'qwen3' in the merged version. The new test uses Qwen/Qwen3.6-35B-A3B, which matches either marker, so the broadening to all Qwen families is untested. Issue #4285 specifically reports the bug for Qwen3.6 + vLLM ≥ 0.20, and the older Qwen2.x family predates the reasoning_content convention, so widening the marker without a concrete user report and a Qwen2-family test isn't motivated by the linked issue.
1. Superseded by already-merged PR #4294 (severity: high · confidence: very high)
The mirroring logic already lives on main (merged in #4294). Rebasing this branch produces a duplicate code path that runs after the upstream mirror has already populated the field, contributing no behavior. If the broader marker is wanted, that's a small follow-up against the existing function rather than a re-add of the whole pipeline.
Verdict
COMMENT — recommend closing as superseded by merged #4294. If qwen2.x coverage is wanted, raise a focused follow-up against the function on main.
|
Closing as superseded by merged #4294, which lands the same fix in the same file with a stricter |
3 participants
Summary
reasoning_contenttoreasoningfor self-hosted Qwen history messages.Testing
Fixes #4285